New DTLS-SRTP implementation based upon SharpSRTP #1486
Conversation
|
No probs at all. This is tricky stuff. I'll pull the changes and run the tests again. In theory most of the WebRTC implementations should work with both RSA and ECDSA but in the past I have observed some only support one with the shift being from RSA to ECDSA. |
|
FireFox now working for the get started app. All but the two docker tests below are now able to get a clean DTLS connection. aiortc test: Exception on sipsrocery client: sipsorcery master test: DTLS handshake succeeeds but then the server (which is sipsorcery master branch code) gets a bunch of unprotect errors. |
|
aiortc test should be fixed. My DigestCertificateUtils.IsHashSupported was returning true only for SHA-256, but the certificate presented by that Docker container was using SHA-512. |
Yes, aiortc test is able to establish a DTLS connection now. |
|
As for the "sipsorcery master test", I think I found the problem and it seems it's a bug in sipsorcery master. DtlsSrtpClient is requesting the use of MKI which it calculates in a strange way: byte[] mki = new byte[(SrtpParameters.SRTP_AES128_CM_HMAC_SHA1_80.GetCipherKeyLength() + SrtpParameters.SRTP_AES128_CM_HMAC_SHA1_80.GetCipherSaltLength()) / 8];
random.NextBytes(mki); // Reusing our secure random for generating the key.
this.clientSrtpData = new UseSrtpData(protectionProfiles, mki);Why is it strange? SrtpParameters.SRTP_AES128_CM_HMAC_SHA1_80.GetCipherKeyLength() returns the key length in bytes already, yet it is being divided by 8 once more. This produces number 3, which means the client will generate MKI the size of 3 bytes. Most implementations I've seen so far were using fixed size of 4 bytes or less, this being 3 is rather uncommon. Second, this MKI is sent to the server to negotiate DTLS-SRTP, which means the server is supposed to use it in all SRTP/SRTCP messages to identify the master keys being used. This is actually what is happening, I can see the server is appending the MKI right before the auth tag (see https://www.rfc-editor.org/rfc/rfc3711#section-3.1). Now the SRTP authentication is failing in sipsorcery master (error code -1) and it is because the Unprotect implementation does not understand the optional MKI in the message (which it had negotiated in the DTLS UseSrtp extension) and it is including it in the authenticated portion of the packet, which is wrong according to RFC 3711. Those 3 bytes should be stripped from the message along with the auth tag like so: // get original authentication and store in tempStore
pkt.ReadRegionToBuff(pkt.GetLength() - tagLength, tagLength, tempStore);
pkt.shrink(tagLength + (SrtpParameters.SRTP_AES128_CM_HMAC_SHA1_80.GetCipherKeyLength() + SrtpParameters.SRTP_AES128_CM_HMAC_SHA1_80.GetCipherSaltLength()) / 8);
// save computed authentication in tagStore
AuthenticatePacketHMCSHA1(pkt, guessedROC);The best fix for the current master branch would be to not negotiate MKI at all (like most of the other WebRTC implementations I've seen) and set it to empty array: byte[] mki = new byte[0];
this.clientSrtpData = new UseSrtpData(protectionProfiles, mki);This should fix the errors. |
Thanks for the analysis. So it works on master now because the DTLS server implementation ignores the mki and doesn't send it to the client? It seems a bit strange that the current master DTLS client implementation, that is setting the mki, doesn't get the errors with any of the other WebRTC implementations. Shouldn't they also be sending the mki back to the DTLS client and would thus trigger the same error message? |
|
"It seems a bit strange that the current master DTLS client implementation, that is setting the mki, doesn't get the errors with any of the other WebRTC implementations."
So I suppose if those other implementations are pure WebRTC, then they are correctly ignoring the MKI in the client's request. Nonetheless I think the client should not have asked for MKI according to the RFC 8827, so it is also a bug of the current sipsorcery master. As for SharpSRTP, it simply implements SRTP and DTLS-SRTP, not WebRTC. However, I think I might offer some way of telling the server to ignore any MKI to make the SRTP server behave as the other WebRTC implementations... Edit: I added ForceDisableMKI property and set it to true in sipsorcery, making it ignore MKI like the other WebRTC implementations. |
|
Thanks for providing that reference, that's great. I've created #1489 to remove MKI. It's trivial but perhaps if you could verify it's all that's required I'd appreciate it. |
|
I've tested with the mki fix and the test with master as the server and this PR as the client for the echo test now works without errors (not the DTLS roles are the reverse of the echo test ones). I've also run the tests with all the echo test client options from the webrtc-interop repo. They all work well except the sipsorcery master branch one which does successfully neogtiate DLS connection but then throws the exception below when closing. The test commands were: Echo test server - webrtccmdline from this PR branch Echo test client (image built from sipsorcery master version 10.0.2): |
|
Looks like an issue in the sipsorcery master to me, at least partially. One part of the problem were the NULL cipher suites. My client/server offered them as supported and they were incorrectly selected by the server (sipsorcery master), which exposed a bug in my code where NULL cipher suites failed to derive the master key/salt. This problem is now fixed and NULL cipher suites seem to be working when negotiated. However, just fixing it was not enough - according to RFC 8827:
I fixed this on my side where I disabled NULL cipher suited for both the client and the server. However, the fact that they were selected by the server despite being offered last led me to finding another bug in the current sipsorcery master. ProcessClientExtensions in DtlsSrtpServer should be modified to pick the first supported cipher suite, not the last one. The cipher suites are listed from the most preferred one to the least preferred one as per RFC 5764:
I think that the code in DtlsSrtpServer.ProcessClientExtensions in sipsorcery master branch should look as follows: // profiles are listed in descending order of preference https://datatracker.ietf.org/doc/html/rfc5764#section-4.1.1
bool isProfileSelected = false;
foreach (int profile in clientSrtpData.ProtectionProfiles)
{
switch (profile)
{
case SrtpProtectionProfile.SRTP_AES128_CM_HMAC_SHA1_80:
case SrtpProtectionProfile.SRTP_AES128_CM_HMAC_SHA1_32:
chosenProfile = profile;
isProfileSelected = true;
break;
}
if (isProfileSelected)
{
break;
}
}This would ensure the first matching profile is chosen as well as all NULL profiles are skipped. |
|
Thanks again for the awesome diagnosis. I'll do some testign with that suggestion. |
|
I've created #1492 to fix the null profiles and incorrect selection logic. If you have a chance to review and confirm that'd be great. |
|
All my tests are now good. I expect there will be a few more edge cases that come out but if you're happy to merge I'm good to go. |
|
Thank you very much for running all the tests and helping me to improve the code! Yes, I'd expect a few more edge cases as well, especially the ones related to the now deprecated old DTLS 1.0. I think we can merge it now and I'll keep looking for new issues being reported here. |
|
All green on the interop tests with the other WebRTC implementations post merge! 
|
| #if NETCOREAPP2_1_OR_GREATER || NETSTANDARD2_1_OR_GREATER | ||
| public void Send(ReadOnlySpan<byte> buf) | ||
| { | ||
| OnDataReady?.Invoke(buf.ToArray()); |
There was a problem hiding this comment.
Besides the usage of the expensive Enumerable.ToArray(), shouldn't this be slicing to off..int?
Something like:
| OnDataReady?.Invoke(buf.ToArray()); | |
| OnDataReady?.Invoke(buf.AsSpan(off, len).ToArray()); |
There was a problem hiding this comment.
In general yes, you are right, but looking at the stack trace it seems it's always used with 0-length which is why it works "as is". However, I agree that if somebody else were to call this method, he might be surprised of the current behavior.
|
|
||
| public DtlsTransport Transport { get; private set; } | ||
| private ConcurrentQueue<byte[]> _data = new ConcurrentQueue<byte[]>(); | ||
| private string _remoteEndPoint; |
There was a problem hiding this comment.
Not used.
| private string _remoteEndPoint; |
| { | ||
| _chunks.Add(buf); | ||
| } | ||
| _remoteEndPoint = remoteEndPoint; |
There was a problem hiding this comment.
Not used.
| _remoteEndPoint = remoteEndPoint; |
There was a problem hiding this comment.
Ah! That's a leftover from the HelloVerify request, which has been disabled for WebRTC because Firefox did not support it. I'll remove it, thank you!
3 participants
To remove the AGPL-derived code, I implemented a new DTLS-SRTP layer based upon BouncyCastle DTLS samples and the RFCs. More info here: https://github.com/jimm98y/SharpSRTP
This PR wires up the new implementation into the current code base. The only modification when compared to SharpSRTP are the namespaces which have been adjusted to match sipsorcery and the logging was wired up to sipsorcery's logging mechanism.
I have unit tests for all the implemented RFCs in my repo and it should be easy to sync any changes/bugfixes in the future. I tested some of the WebRTC samples that I was able to run on ARM64 and I also tested the new implementation on https://github.com/jimm98y/SharpRTSPtoWebRTC.
This new DTLS-SRTP implementation also fixes the RSA TLS certificate sipsorcery was using in WebRTC, now it's using ECDsa by default. By default I also disabled DTLS 1.0, leaving only DTLS 1.2 available. DTLS 1.0 can be enabled using overrides. DTLS 1.3 currently cannot be supported because BouncyCastle does not support it yet.
This PR should also resolve AES-GCM AEAD support #871.